fix(jdbc-v2): render array contents for named tuples - #3046

@claude review

Claude finished @mshustov's task in 4m 7s —— View job

Review: fix(jdbc-v2): render array contents for named tuples

• Read repo review guidelines (docs/ai-review.md, AGENTS.md, docs/features.md)
• Examine the diff and surrounding code
• Analyze changes for correctness, compatibility, and test quality
• Post review feedback

Findings (ordered by severity)

1. The output format is a one-way compatibility decision — confirm deepToString is the format you want to ship. (design/compatibility)

Arrays.deepToString produces Java-style rendering, not ClickHouse literal syntax: Array(Tuple(id UUID)) renders as [[550e8400-…]] rather than [('550e8400-…')]. Consequences worth a deliberate maintainer decision before this is released, since per docs/ai-review.md toString() output is compatibility-sensitive and changing it later would be a breaking change of formatted output:

• Tuples and nested arrays are indistinguishable — Array(Tuple(UUID)) and Array(Array(UUID)) both render as [[…]].
• Strings are unquoted and unescaped, so the output is ambiguous: ['a, b'] and ['a', 'b'] both render as [a, b].
• Common JDBC practice (e.g. pgjdbc's PgArray#toString) is to render the database's own literal form. If you ever want ClickHouse-literal output (which would round-trip and match getString()-style server rendering), it's cheaper to do it now than after clients start depending on this format.

If readable-but-approximate output is the intended contract (which is a defensible reading of #3045 — the goal is fixing Array@54c3b772 in IDE viewers), this is fine as-is; the new tests correctly lock the format in. Just flagging that this PR is the moment the format gets frozen. Fix this →

2. Struct (top-level Tuple(...) columns) still has the same display problem. (consistency)

com.clickhouse.jdbc.types.Struct (Struct.java) does not override toString(), so SELECT ('…')::Tuple(id UUID) (without the array) still displays as com.clickhouse.jdbc.types.Struct@… in the same IDE viewers. Fine as a follow-up to keep this PR focused, but worth an issue so array and tuple rendering don't diverge — and it's another reason to settle finding 1 first, since Struct#toString should use the same format.

3. toString() after free() returns the string "null". (minor)

free() nulls the array field (Array.java:120-123), and Arrays.deepToString(null) returns "null". No NPE, and toString() shouldn't throw SQLException, so this behavior is acceptable — just noting it's a deliberate trade-off, and a one-line unit-test case would lock it in.

4. New single-test integration class instead of an existing home. (style, minor)

ArrayTupleIntegrationTest introduces a new class (and a naming pattern — other integration tests here are *Test/*Tests, e.g. JdbcDataTypeTests, with groups = {"integration"}) for one test method. Folding it into JdbcDataTypeTests or ResultSetImplTest would match existing conventions. The unit ArrayTest is fine — it's the natural new home for types.Array unit tests.

What's good

• The fix is the smallest safe change: only toString() is added; getArray(), return types, and all JDBC semantics are untouched. I verified getString() on an array column goes through the client-v2 reader (ResultSetImpl.getString → reader.getString), and getObject(col, String.class) converts the raw reader value — neither routes through this wrapper's toString(), so the PR's "display-only" compatibility claim holds.
• Arrays.deepToString correctly handles nested Object[] tuples, nested primitive arrays, and self-referential arrays.
• Both a unit test and an integration test through the real read path (named and unnamed tuples), matching the repo's preference for integration coverage; tests are clean of issue-number narration.
• CHANGELOG entry is in the right section with the issue link.

Docs

docs/features.md line 90 documents the jdbc-v2 arrays/tuples feature; per docs/ai-review.md, an intentional user-visible behavior change should be reflected there. Suggest adding a short clause that Array#toString() renders nested contents (deep formatting).

Tests run

I could not run Maven locally (the mvn command is not in this run's allowed tools); PR CI (Build, Coverage) was still in progress at review time. The tests are simple and read as correct against the verified real-path behavior.

Summary

• What changed: jdbc-v2 Array gains a toString() override using Arrays.deepToString, plus unit/integration tests and a CHANGELOG entry.
• Compatibility risk: Low. No API or return-type changes; the only externally visible change is toString() output, which previously was the unusable Object#toString default. The chosen format itself becomes a compatibility commitment (finding 1).
• Docs: recommend a one-line docs/features.md update.

Verdict: ready for human review

--- · Branch

@014-code
Thank you for the contribution!
However initial issue seems need more research. I do not want to make toString() traverse deep arrays. It may be another issue.

@014-code Thank you for the contribution! However initial issue seems need more research. I do not want to make toString() traverse deep arrays. It may be another issue.

Thank you for the clarification.

I have withdrawn the Array#toString() deep-formatting approach, including its tests and changelog entry. I agree that traversing nested arrays from toString() may affect a hot path, and the resulting representation would not establish a sufficiently clear or controllable JDBC display contract.

I also merged the latest main into this branch and pushed the update.

I will close this PR for now. The original behavior in #3045 needs further investigation and a separately agreed approach before introducing any array or tuple display formatting.